LGTM

Amusingly btw, the names for strWrapper and bufWrapper were reversed:

• bufWrapper was used for strings, and its internal comment referred to strings
• strWrapper was used for Buffer, and its internal comment referred to Buffer

Anyway, that doesn't matter anymore as they've been unified now.

CI Run: https://ci.nodejs.org/job/node-test-pull-request/339/

I think this was actually done on purpose to avoid polymorphism

@vkurchatkin Could you please elaborate?

@thefourtheye FWIW this blog post explains mono/poly/mega-morphism (how it relates to v8/JavaScript) pretty well.

I think this was actually done on purpose to avoid polymorphism

Correct me if I'm wrong:
That does only apply if you access any property of the object, doesn't it?
Here we are just holding a reference and pass it to the callback -> no property access -> no polimorphism

After looking at FSWrapReq, it looks like the function arguments are always the same types for a write operation, so I think @meinklaus is right here.

Any blockers?

LGTM if the CI is happy. For the record, I don't even know if keeping a reference to the string is necessary ("external" strings? this isn't handled elsewhere afaik).

I don't know either to be honest. @trevnorris may know more on this. I don't see that as a blocker for this PR, but if we can get clarity on that, of course I can update the PR if needed. I wouldn't want to introduce risk though.

IIRC you had a fix like this before. LGTM. I'm the one who did it this way. Was b/c I was overly strict about where I placed that assignment. Wasn't actually needed.

In the previous PR it was about buffers, this time it's about a string (well, the conversation is). Are you saying I can remove buffer from that wrapper altogether? Or only in the case of strings? Or am I misunderstanding? In any case, it does seem like we need the wrapper to ensure we return the number of bytes written (although I have a hard time believing that C++ land won't return a number as the 2nd argument, but I can look into that).

I'm on crack. Was referring to changing the location of req.oncomplete = wrapper;

As for maintaining a reference to the string, when the string is externalized a reference has to be kept. Since we used the external memory from the string object. So if the string is GC'd while the data is written it can blow up.

Got it, thanks. Sounds like we're all good then :)

Yup. LGTM

Looking forward to seeing it merged :) Anything I can do to speed that up?

Whoops. Sorry. Running CI to verify everything is fine: https://ci.nodejs.org/job/node-test-pull-request/627/

Bugger. NM. ^F failed me, didn't see the CI run that @thefourtheye posted above.

Landed in c339fa3. Thanks much!

@trevnorris @thefourtheye @vkurchatkin ... should this also go into v4.x?

@jasnell code cleanup, no bug fix but also no side-effects. might be useful to have if landing future patches in the same code in the future.

Landed in v4.x-staging in fb3b848